Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard: automatically reload mapping file and update tooltips #13082

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 10, 2024

Yeeiij, finally!
Tooltips and the mapping are rebuilt completely if the mapping file has been changed (or removed).
As before, we look for the user's Custom.kbd.cfg first, then use the built-in mapping based on the selected locale.

This 'only' covers the GUI mapping. Extending this to WMainMenuBar, as done in #1746, is certainly possibly. done ✔️

Inspired by #1746, but implemented more simply.
Replaces #13074

Fixes #9814

@github-actions github-actions bot added the ui label Apr 10, 2024
@ronso0 ronso0 added this to the 2.5-beta milestone Apr 10, 2024
@ronso0
Copy link
Member Author

ronso0 commented Apr 10, 2024

oh, need to consider skin reload...

edit: Done.

@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch 2 times, most recently from c41be0a to 6896dc8 Compare April 11, 2024 23:03
@ronso0 ronso0 marked this pull request as ready for review April 14, 2024 22:29
@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 6896dc8 to 428caf9 Compare April 15, 2024 18:54
@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2024

Extending this to WMainMenuBar, as done in #1746, is certainly possibly.

Done.

@ronso0
Copy link
Member Author

ronso0 commented Apr 16, 2024

We could add dummy shortcuts (empty) to all currently unmapped menubar actions so users can add shortcuts themselves.
(i.e. add ConfigKeys to menubar and all *.kbd.cfg files)

Nothing that holds up this PR though.

Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too important, but the upper/lowercasing of KeyboardEventFIlter got a bit mixed up the commit message Keyboard: handle mapping file and config setting in KeyboardEventFIlter`.

src/widget/wmainmenubar.cpp Outdated Show resolved Hide resolved
@daschuer
Copy link
Member

daschuer commented May 1, 2024

This one has a conflict now. Can we remove the 2.5-beta milestone?

@ronso0 ronso0 marked this pull request as draft May 1, 2024 15:20
@ronso0 ronso0 modified the milestones: 2.5-beta, 2.5.0 May 1, 2024
@ronso0
Copy link
Member Author

ronso0 commented May 1, 2024

Sure, assigned it to 2.5 for now because I think this is a bugfix, see #9814
It can either slip in during the beta period (it's already working without issues), or later.

Set to draft because I've just rebased this and am about to tweak it considering @cr7pt0gr4ph7's suggestions.

@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 4583b2d to 61db431 Compare May 1, 2024 16:47
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick: Spelling of "chnanged" in the commit message. LGTM otherwise.

@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 61db431 to 0b63ef6 Compare May 3, 2024 10:25
@ronso0
Copy link
Member Author

ronso0 commented May 3, 2024

Thanks for taking the time to review!

Spelling of "chnanged" in the commit message.

Yeah, that happens when I work in bright sunlight...

@ronso0 ronso0 marked this pull request as ready for review May 3, 2024 10:26
@daschuer daschuer modified the milestones: 2.5.0, 2.5-beta May 8, 2024
@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 0d83677 to 44f47e9 Compare October 13, 2024 14:18
@ronso0
Copy link
Member Author

ronso0 commented Oct 13, 2024

Oh sorry, I did a rebase instead of merging main.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple codestyle complaints

src/skin/legacy/legacyskinparser.cpp Outdated Show resolved Hide resolved
src/skin/legacy/legacyskinparser.h Outdated Show resolved Hide resolved
src/coreservices.h Outdated Show resolved Hide resolved
src/widget/wbasewidget.h Outdated Show resolved Hide resolved
src/widget/wbasewidget.h Outdated Show resolved Hide resolved
src/widget/wmainmenubar.cpp Outdated Show resolved Hide resolved
src/widget/wmainmenubar.cpp Outdated Show resolved Hide resolved
src/widget/wmainmenubar.cpp Outdated Show resolved Hide resolved
src/controllers/keyboard/keyboardeventfilter.cpp Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mostly copy pasted, right? Do you want to improve it or leave it?

@ronso0
Copy link
Member Author

ronso0 commented Oct 23, 2024

some thoughts on the general tooltip update approach used here and performance:

With the current implementatoin kbd mapppings are (re)loaded

  • on startup
  • when kbd shortcuts are toggled on, and
  • when the mapping file has been edited

so I think a tiny performance hit (GUI thread) in these non-critical situations is an acceptable tradeoff for having the GUI reflect the complete kbd mapping (all possible poti sub-controls) immediatly, even if it's just for double-checking that the mapping has been applied as expected.

The current method is:

  • for each control widget (button, knob, ..) the skin parser collects ConfigKeys for the from-widget connections, optionally creates the sub-control strings to be displayed in the tooltip
    ('up', 'down', 'up small', 'down small' for knobs and sliders, 'activate', 'clear' etc. for Hotcue buttons)
  • this CfgKey/string list is stored in each WBaseWidget
  • each widget is registered in KeyboardEventFilter (~1600 for LateNight 16 Samplers)
  • KeyboardEventFilter loads the kbd mapping file
  • iterate over all registered widgets, check if the controls are in the kbd mapping
    • yes: (re)create kbd tooltip strings
    • no: clear kbd tooltip string
    • push string to widget, (re)construct tooltip
  • similar method for menubar actions

The downside:
all widgets are updated even if just one shortcut has been changed.

So I tried another approach, hoping that it would it be faster (in total, ie. load mapping file, check shortcuts, update widgets):

  • reload kbd mapping
  • compare last/new mapping, collect changed/added/removed shortcuts
  • collect affected widgets
  • update affected widgets

I had to implement the compare method and store more data in KeyboardEventFilter, and it turned out that this did not shorten the total time to the extent that it justifies the additional complexity (and potential errors/inconsistency), given that shortcuts are evaluated rarely.
(current method: ~6-7ms in total, alternative: ~5-6ms, measured when changing one shortcut)
I can provide the commit if someone is interested.

@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 0a3d391 to 465ba9a Compare October 24, 2024 00:13
@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2024

some thoughts on the general tooltip update approach used here and performance:

With the current implementatoin kbd mapppings are (re)loaded

  • on startup
  • when kbd shortcuts are toggled on, and
  • when the mapping file has been edited

so I think a tiny performance hit (GUI thread) in these non-critical situations is an acceptable tradeoff for having the GUI reflect the complete kbd mapping (all possible poti sub-controls) immediatly, even if it's just for double-checking that the mapping has been applied as expected.

The current method is:

  • for each control widget (button, knob, ..) the skin parser collects ConfigKeys for the from-widget connections, optionally creates the sub-control strings to be displayed in the tooltip
    ('up', 'down', 'up small', 'down small' for knobs and sliders, 'activate', 'clear' etc. for Hotcue buttons)
  • this CfgKey/string list is stored in each WBaseWidget
  • each widget is registered in KeyboardEventFilter (~1600 for LateNight 16 Samplers)
  • KeyboardEventFilter loads the kbd mapping file
  • iterate over all registered widgets, check if the controls are in the kbd mapping
    • yes: (re)create kbd tooltip strings
    • no: clear kbd tooltip string
    • push string to widget, (re)construct tooltip
  • similar method for menubar actions

The downside:
all widgets are updated even if just one shortcut has been changed.

So I tried another approach, hoping that it would it be faster (in total, ie. load mapping file, check shortcuts, update widgets):

  • reload kbd mapping
  • compare last/new mapping, collect changed/added/removed shortcuts
  • collect affected widgets
  • update affected widgets

I had to implement the compare method and store more data in KeyboardEventFilter, and it turned out that this did not shorten the total time to the extent that it justifies the additional complexity (and potential errors/inconsistency), given that shortcuts are evaluated rarely.
(current method: ~6-7ms in total, alternative: ~5-6ms, measured when changing one shortcut)
I can provide the commit if someone is interested.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2024

IMO this is now ready for a final review.
@Swiftb0y suggestions for improving the copied code (eventFilter()) are welcome!

@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from 94f3037 to e9f3ca0 Compare October 26, 2024 12:03
@ronso0 ronso0 force-pushed the kbd-mapping-reload-tooltip-update branch from e9f3ca0 to c85011c Compare October 26, 2024 12:07
@ronso0 ronso0 mentioned this pull request Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show keyboard shortcuts in tooltip instantly after enabling shortcuts
6 participants